-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: calculate total seconds from interval fields for extract(epoch)
#19807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Jefffrey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Results seem consistent with DuckDB 👍
|
|
||
| fn epoch(array: &dyn Array) -> Result<ArrayRef> { | ||
| const SECONDS_IN_A_DAY: f64 = 86400_f64; | ||
| const DAYS_PER_MONTH: f64 = 30_f64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose there's no easier way to define this for intervals 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I just followed the PostgreSQL-way for simplicity here, what do u think?
https://doxygen.postgresql.org/datatype_2timestamp_8h.html#ad35c6d425de4ccc4718c6ce7f4bfbba2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing! Maybe we could add some more test cases? E.g. extract(epoch from arrow_cast( '100ms'::interval, 'Interval::(MothDayNano)') -> 0.1` and such.
7d2c798 to
d73f19a
Compare
Which issue does this PR close?
select extract(epoch from ('15 minutes')::interval)gives wrong result #19799.Rationale for this change
The epoch function incorrectly used
date_part(array, DatePart::Second)for intervals which only extracts the seconds component of the interval structure (0 for "15 minutes"), not the total seconds from all components.What changes are included in this PR?
Are these changes tested?
Yes. Added tests verifying correct conversion for all interval types and precision levels.
Are there any user-facing changes?
Yes.
extract(epoch from interval)now returns correct total seconds instead of 0.